-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added BB step size rule to our step size methods #1859
Added BB step size rule to our step size methods #1859
Conversation
Thank you, I can take a look. Could you: Clarify what is plotted on the y-axis, maybe better show y as log, or loglog, and I don't see the green one? |
I wasn't aware there so many variants and stabilization methods for BB. We also used BB some years back for a smoothed TV regularization problem and compared with Nesterov acceleration https://doi.org/10.1007/s10543-011-0359-8 Do you see if these BB methods are related? |
Thanks @jakobsj, I have updated those plots so hopefully they are a bit clearer |
This could be an option, instead of the "stabilisation" procedure that I use with an option for the user to turn it on or off. I think it was introduced in Raydan, M.: The Barzilai and Borwein gradient method for the large scale unconstrained minimization problem. SIAM J. Optim. 7, 26–33 (1997) but I am having trouble accessing it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really great addition and as the experiments demonstrate big speed-up improvements can often be achieved. Nice extensive tests. I added some requests mostly on further elaborating on the documentation and choice of default behaviours.
Oh and many thanks for comparing. I think no reason to add the backtracking instead of the stabiliser. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost everything nicely addressed, I resolved all comments, and only left one new comment about the None/inf input thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! "automatic" is really clear and better than the suggested "default". Approving as is - could consider if "auto" instead of "automatic" would be (almost) equally clear/commonly used and briefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing @lauramurgatroyd - looks better |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor changes left. And happy to discuss if you disagree.
Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
See updated documentation #1859 (comment)
Changes
Adds the BB long and short (from https://en.wikipedia.org/wiki/Barzilai-Borwein_method) step size rule
Adds the stabilisation (adaptive and non-adaptive) from Burdakov, O., Dai, Y.H. and Huang, N., 2019. Stabilized barzilai-borwein method. arXiv preprint arXiv:1907.06409.
Testing you performed
For a limited angle parallel beam leastsquares +Tikhonov reconstruction using GD the comparison of step size methods is below:
For a random matrix Ax=b problem, with a leastsquares objective, reconstruction using GD the comparison of step size methods is below:
For these couple of examples "SHORT" is the most stable. Removing the stabilisation has the potential for very fast convergence but is unstable (and not guaranteed to converge!)
Related issues/links
Closes #748
Checklist
Contribution Notes
Please read and adhere to the developer guide and local patterns and conventions.